Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better capacity and growth rate for {Stream,Future}Group #172

Closed
wants to merge 12 commits into from

Conversation

matheus-consoli
Copy link
Collaborator

@matheus-consoli matheus-consoli commented Mar 22, 2024

Fixes the same problem as #168 by keeping up with the capacity and growth rate of the group.

When a group tries to grow over its capacity, we double the previous capacity.

FutureGroup and StreamGroup are structurally very similar, they only differ on the Stream implementation, so I isolated the common structure to reduce the duplicated code.

Performance

Compared to the main branch, we gained 99,775% using this benchmark (with 1_000_000 futures).

Before:

group/futures_concurrency::FutureGroup
                        time:   [27.222 s 28.561 s 29.720 s]

After:

group/futures_concurrency::FutureGroup
                        time:   [68.507 ms 68.598 ms 68.752 ms]
                        change: [-99.769% -99.760% -99.748%] (p = 0.00 < 0.05)
                        Performance has improved.

todos

  • make FutureGroup and StreamGroup type aliases for a common struct generic over the poll behavior
  • rename resize to reserve and expose it
  • tests and docs and benchmarks?

Closes #169

@matheus-consoli matheus-consoli marked this pull request as ready for review March 24, 2024 17:28
@yoshuawuyts
Copy link
Owner

@matheus-consoli apologies for taking a little while to respond; I was out all of last week. I love that you've implemented a more robust version of #168, and I'm 100% on board with solving #169 the way you've done it. However, I'm unsure whether using the InnerGroup implementation you've provided here is the way to do it. Let me try and explain.

In the current implementation we have two structs:

  • StreamGroup for T: Stream
  • FutureGroup for T: Future

In the implementation provided by this PR we have a hierarchy of structs, at the top level there is:

  • StreamGroup for T: Stream
  • FutureGroup for T: Future

Which internally uses a shared impl:

  • InnerGroup<B: PollBehavior>

PollBehavior here abstracts over the differences between Stream and Future, and is in turn implemented on two additional structs:

  • impl PollBehavior for PollFuture
  • impl PollBehavior for PollStream

My worry is that this creates a funnel-shaped abstraction: we're specializing behavior at the top of the abstraction, going through a single shared type, and then specializing again at the bottom of the abstraction. I worry that this may work because poll_next is almost identical to poll, but if we wanted to add something more left-field such as AsyncIteratorGroup this layering would break down.

I think for that reason we might be better served with just duplicating the strategy between both impls. And perhaps we can explore ways to share more logic between impls without specializing at the lower levels in future PRs? Would that be ok with you?

@matheus-consoli
Copy link
Collaborator Author

sure, i agree with you

my initial idea was to write a single type InnerGroup and have {Future, Stream}Group be type aliases (that would, more or less, break the funnel-shaped problem), but i also didn't want to expose those in the public API... and i wasn't considering an AsyncIteratorGroup.

I will close this PR and open a simpler one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement better capacity in our group structures
2 participants